Fix java/tainted-arithmetic CodeQL alerts - #765
Conversation
Use Integer.compare instead of subtraction in CSN.compare: seqnum and serverId come from replication messages and the subtraction could overflow, inverting the comparison sign. Compute the new position in ByteSequenceReader.skip in long arithmetic to avoid int overflow, and validate the parsed fraction range in GeneralizedTime before scaling.
vharseko
added
security
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
The CSN.compare() fix is correct and more valuable than the description suggests — the real hazard isn't a flipped sign but comparator non-transitivity, and CSN is a TreeMap key in MsgQueue, PendingChanges, RemotePendingChanges, LDAPReplicationDomain.replayOperations and EntryHistorical:
seqnum MIN_VALUE / 0 / MAX_VALUE at equal timestamps, old code:
MIN < 0 0 < MAX but MIN - MAX = +1 => MIN > MAX
Reachable only via CSN.valueOf(ByteSequence) (raw readInt() off the replication wire / changelog) — CSNGenerator guards ++seqnum <= 0, and CSN(String) can't parse one (Integer.parseInt("80000000", 16) throws). Verified: no sign change for non-negative seqnums, and every call site uses only the sign.
Two things to fix in GeneralizedTime before merge.
GeneralizedTime: the "Cannot happen" comment is wrong — the branch is live (Medium)
Double.parseDouble rounds: "0." + 17 nines parses to exactly 1.0. The branch is reachable and load-bearing. Measured at the schema layer (GeneralizedTimeSyntaxImpl.valueIsAcceptable, which catches only LocalizedIllegalArgumentException and never calls getTimeInMillis()):
20240101000000.<n nines>Z |
base | this PR |
|---|---|---|
| 17 nines | schema-accepted, then IllegalArgumentException: MILLISECOND later |
rejected at schema check |
So the change is a real fix, not an assertion. Please correct the comment and the PR body — as written, someone will delete a live branch as dead code.
GeneralizedTime: the guard checks fractionValue, not the value actually used (Medium)
fractionValue < 1.0 doesn't bound additionalMilliseconds. With 16 nines the guard passes but the rounded result is still out of range, so the value survives schema validation and detonates later as an unlocalized IllegalArgumentException — outside the method's declared contract, because the Calendar is evaluated lazily in getTimeInMillis():
Math.round(0.9999999999999999 * 1000) == 1000 // legal Calendar.MILLISECOND is [0,999]
16 nines, base: schema-accepted -> later IllegalArgumentException: MILLISECOND
16 nines, PR: schema-accepted -> later IllegalArgumentException: MILLISECOND <- unchanged
Guarding the value actually consumed catches both cases:
final double fractionValue = Double.parseDouble(fractionBuffer.toString());
final long additionalMilliseconds = Math.round(fractionValue * multiplier);
if (additionalMilliseconds < 0 || additionalMilliseconds >= multiplier) {
throw new LocalizedIllegalArgumentException(
WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME.get(value, fractionBuffer));
}Pre-existing, but it's the same defect class the PR is closing, one line away.
No tests for three behaviour-relevant changes (Low)
opendj-server-legacy/src/test/java/org/opends/server/replication/common/CSNTest.java—compare()with negative/extreme seqnums plus a transitivity assertion. Line 305 already usesInteger.MAX_VALUE-1, andcompareToEquivalentToEquals(line 337) is data-driven, so both are cheap to extend.opendj-core/src/test/java/org/forgerock/opendj/ldap/GeneralizedTimeTest.java— the 17-nines value is now rejected where it previously wasn't. That's the only thing keeping the comment honest.
Nits
ByteSequenceReader.skip()is behaviourally a no-op: withpos ∈ [0, 2³¹-1]andlength ∈ [0, 2³¹-1]the sum tops out at2³²-2, so overflow always wraps into[-2³¹, -2]— negative, whichposition()already rejected. A sweep of the input space found zero old-vs-new divergences in accept/reject. Fine to keep (silences the alert, documents intent), but "pos + lengthcan no longer overflowint" in the PR body implies a defect that wasn't there.- The
serverIdhalf of theCSNchange is cosmetic: both decode paths bound it to[0, 65535](readShort() & 0xffff; 4 hex chars), so that subtraction could never overflow. Harmless and consistent, just not a fix. - Copyright wording:
Portions Copyrighted 2026— repo convention isPortions Copyright 2026 3A Systems, LLC.(74 occurrences vs 9). Not enforced; the plugin is commented out inpom.xml:56. - Header applied inconsistently:
GeneralizedTime.javaandCSN.javagot the line,ByteSequenceReader.javawas modified but didn't. - Message reuse:
WARN_ATTR_SYNTAX_GENERALIZED_TIME_ILLEGAL_TIME's second placeholder is an exception/reason everywhere else; passingfractionBufferrenders as "…represents an invalid time (e.g., a date that does not exist): 0.99999999999999999". Serviceable, slightly off-register.
Out of scope
finishDecodingFraction sets the scaled fraction into Calendar.MILLISECOND instead of adding it to the time, so all fractional-minute and fractional-hour values are broken, not just edge cases:
20240101000000.5Z OK millis=1704067200500
202401010000.5Z IllegalArgumentException: MILLISECOND
2024010100.5Z IllegalArgumentException: MILLISECOND
2024010100.001Z IllegalArgumentException: MILLISECOND
Pre-existing and unrelated — worth a separate issue. It also means the new guard only ever matters on the seconds path.
- GeneralizedTime: guard the value actually consumed - Math.round(fraction * multiplier) must stay in [0, multiplier) - instead of the parsed fraction: parseDouble rounds "0." + 17 nines to exactly 1.0, and 16 nines scale and round to 1000 ms; both previously passed schema validation and failed later in getTimeInMillis() with an unlocalized IllegalArgumentException. - Tests: GeneralizedTimeTest rejects 16/17-nines fractions, CSNTest covers extreme-seqnum ordering and transitivity, ByteSequenceReaderTest covers skip() overflow. - Align header wording with repo convention (Portions Copyright).
|
Thanks for the thorough review — all points addressed in 2d46e75. Guard now checks the consumed value. Tests added:
Headers: wording aligned to the repo convention ( PR description updated: the On the message nit: kept Agreed that the fractional-minutes/hours defect in Verified locally: |
The range check ran after the multiplication, so the merge-ref analysis re-raised java/tainted-arithmetic on the same expression (alert 1225) and flagged the now-moved Double.parseDouble with java/uncaught-number-format-exception (alert 1226). Guard the parsed fraction to [0, 1) before scaling and catch NumberFormatException with the documented LocalizedIllegalArgumentException. The post-round check remains for the 16-nines case, where 0.9999999999999999 * 1000 still rounds to 1000.
3 participants
Fixes all four open
java/tainted-arithmeticCodeQL code-scanning alerts (High): alert 111, alert 112, alert 113, alert 114.CSN.compare()(alerts 113, 114): replaced subtraction-based comparison withInteger.compare().seqnumarrives as a rawreadInt()from replication messages/changelog (CSN.valueOf(ByteSequence)); with extreme operands the subtraction overflows and breaks comparator transitivity (MIN_VALUE < 0 < MAX_VALUE, butMIN_VALUE - MAX_VALUE == +1), which is fatal for theTreeMaps keyed onCSN(MsgQueue,PendingChanges,EntryHistorical, ...). TheserverIdhalf is consistency only: both decode paths bound it to[0, 65535], so that subtraction could not overflow. Normal ordering and equality are unchanged.GeneralizedTime(alert 112): the parsed fraction is validated after scaling —Math.round(fraction * multiplier)must stay in[0, multiplier). This is a live check, not an assertion:"0."+ 17 nines parses as exactly1.0, and 16 nines scale and round to1000ms; both previously passed schema validation and detonated later ingetTimeInMillis()with an unlocalizedIllegalArgumentException(thelenient=falsecalendar validates lazily). Both are now rejected at decode time with the documentedLocalizedIllegalArgumentException.ByteSequenceReader.skip()(alert 111): the new position is computed inlongarithmetic with an explicit bounds check. Behaviorally equivalent to the old code (an overflowedpos + lengthalways wrapped negative, whichposition()already rejected) — the change silences the alert and documents intent.Tests:
GeneralizedTimeTest: 16- and 17-nines fraction values added toinvalidStrings— both are now rejected at schema-validation time.CSNTest:csnCompareExtremeSeqnums(sign and transitivity forMIN_VALUE/0/MAX_VALUEseqnums at equal timestamps), extreme seqnums added to thecreateCSNpair-comparison data provider.ByteSequenceReaderTest:testSkipOverflow—skip(Integer.MAX_VALUE)past the end must throwIndexOutOfBoundsException, not wrap around.